Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy load image support for Next theme. #2587

Closed
wants to merge 3 commits into from

Conversation

ivan-nginx
Copy link

@ivan-nginx ivan-nginx commented May 28, 2017

Thank you for creating a pull request to contribute to Hexo code! Before you open the request please review the following guidelines and tips to help it be more easily integrated:

  • Add test cases for the changes.
  • Passed the CI test.

This add compare fir `lazy` classname and if it true, will add 'data-original' attrubute to replace it with `src` attrubute, to make image load lazy.
If no `lazy` class defined, image tag will work at standart state.
@coveralls
Copy link

coveralls commented May 28, 2017

Coverage Status

Coverage decreased (-0.05%) to 97.09% when pulling 0279623 on ivan-nginx:master into b76a7b2 on hexojs:master.

Copy link
Member

@NoahDragon NoahDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the errors in Travis-ci and add test cases.
Moreover, could this change be more generic, like based on config file?

@ivan-nginx
Copy link
Author

ivan-nginx commented May 29, 2017

@NoahDragon

Please fix the errors in Travis-ci and add test cases.

I not sure how do it. Can u show?

Moreover, could this change be more generic, like based on config file?

What are u mean? Need to write something like docs or what?

@coveralls
Copy link

coveralls commented May 29, 2017

Coverage Status

Coverage decreased (-0.05%) to 97.09% when pulling 4a33c1e on ivan-nginx:master into b76a7b2 on hexojs:master.

@ivan-nginx
Copy link
Author

@NoahDragon all possible errors was fixed.
About second question: this is lazy image load not instantly. So, original lazy source need data-original attribute to take saved image and load it later at scroll.
If u may give any suggestion about lazy loading, i listen. But for now i don't see any gooder variants to do this.

@NoahDragon
Copy link
Member

@ivan-nginx Sorry for the late reply. I missed this PR in the pile of notifications.
The test cases could be added into https://github.com/hexojs/hexo/blob/master/test/scripts/tags/img.js

IMO, the string lazy, /images/loading.gif, and etc. are hard coding, which forces others to use the same string, and may break other themes using the same syntax. Probably write those string into config file, and using the config.lazy_load_class config.lazy_load_path to access them.

@ivan-nginx
Copy link
Author

@NoahDragon lazy load path — it's good idea, ok. But lazy load class? This is to big and how about speed generation? For this little option add many other cases?

Can u show me how add test cases? I don't add any cases before.

@NoahDragon
Copy link
Member

@ivan-nginx Could you please follow the samples in the test file https://github.com/hexojs/hexo/blob/master/test/scripts/tags/img.js . It's using the Mocha as the test framework, you could find more info on their website https://mochajs.org/.

@stale stale bot added the wontfix This will not be worked on label Sep 27, 2017
@stale
Copy link

stale bot commented Sep 27, 2017

This issue has been automatically marked as stale because lack of recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

1 similar comment
@stale
Copy link

stale bot commented Nov 26, 2017

This issue has been automatically marked as stale because lack of recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Nov 26, 2017
@NoahDragon NoahDragon closed this Nov 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants